Skip to content

Eliminate word_and_empty methods. #140095

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Apr 21, 2025

To remove the last remaining Ident::empty uses.

r? @jdonszelmann

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 21, 2025
@rustbot
Copy link
Collaborator

rustbot commented Apr 21, 2025

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

@nnethercote
Copy link
Contributor Author

nnethercote commented Apr 21, 2025

An opinionated PR, this one... sorry there are a bunch of changes in a single commit, it was hard to separate them out. Removing the functions that returned tuples was important, they got in the way of other changes.

@jdonszelmann
Copy link
Contributor

I see, well I'll take a look :)

@nnethercote
Copy link
Contributor Author

This helps with #137978.

@jdonszelmann
Copy link
Contributor

hm, would you mind at least making the removals from parser.rs a separate commit. I'm not 100% sure I agree with those removals. I intended those APIs to be for all the future attribute parsers that will be converted to a dedicated parser implementation. The reason I am kind of okay with it is because that some of those APIs were made before I allowed easy matching on attribute parsers, which really is a better API so maybe that's what we should switch to. But that'd still be nicer to discuss/refer to if it were a separate commit. I also quickly want to see whether some of my open but blocked PRs relied on those APIs just to see if I had very good reasons for them but I doubt it.

@nnethercote
Copy link
Contributor Author

I have split it into two commits.

@jdonszelmann
Copy link
Contributor

The first commit is blocking another PR. That's the one I reviewed and am happy with merging (maybe after some nitpicks are addressed). I think some of those unwraps might be unnecessary. I can take one more look tomorrow if you decide to change something and then we can merge it. The other, second, commit I have some doubts about right now, which I expressed earlier. If it helps unblock another PR, feel free to refile the second commit in another PR and assign me, then I'll take a look later when I have more time. Rust Week planning is taking a lot. Good night!

To get rid of the `Ident::empty` uses.

This requires introducing `PathParser::word_sym`, as an alternative to
`PathParser::word`.
By using `@` patterns more.

Also, use `Symbol` more in a couple of errors to avoid some unnecessary
conversions to strings. This even removes a lifetime.
@nnethercote
Copy link
Contributor Author

I have removed the original second commit with the API changes; we can try again with those in a follow-up PR.

I have added a new second commit that avoids the unwraps and does a couple of other small related improvements.

@rustbot ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants